[MOOSE-350] Faceted (FacetWP) Directory#327
[MOOSE-350] Faceted (FacetWP) Directory#327GeoffDusome wants to merge 6 commits intotask/MOOSE-350/add-facetwp-pluginfrom
Conversation
GeoffDusome
requested review from
LayaTaal,
MlKilderkin,
Vinsanity,
dpellenwood,
jamiepaul,
spadilha and
thinkstylestudio
GeoffDusome
added
Needs FE Review
Needs BE Review
Launch Environment
GeoffDusome
changed the base branch from
main
to
task/MOOSE-350/add-facetwp-plugin
…50/faceted-directory
spadilha
left a comment
spadilha
left a comment
There was a problem hiding this comment.
Well done, @GeoffDusome 🌮🌮!! This is a big addition and very well implemented. I left a few comments and suggestions, plus one small thing that should be addressed, but nothing major. Thanks for the great work!
| .fs-option { | ||
| --checkbox-size: 24px; | ||
|
|
||
| /* top and bottom padding = half of the spacer-30 (helps each item have a larger click target) | ||
| left padding = spacer-20 + checkbox size + spacer-10 [label displays here] */ | ||
| padding: calc(var(--spacer-30) / 2) var(--spacer-20) calc(var(--spacer-30) / 2) calc(var(--spacer-20) + var(--spacer-10) + var(--checkbox-size)); | ||
|
|
||
| .fs-checkbox { | ||
| width: var(--checkbox-size); | ||
| height: var(--checkbox-size); | ||
| position: absolute; | ||
| left: var(--spacer-20); | ||
| top: 50%; | ||
| transform: translateY(-50%); | ||
| background: none; | ||
| border: 1px solid var(--color-neutral-30); | ||
| border-radius: 0; | ||
| background-color: var(--color-white); | ||
| transition: var(--transition); | ||
|
|
||
| i { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Please add position: relative; to .fs-option, otherwise .fs-checkbox may overlap when the facet “Multi-select” is disabled. Even better might be to remove or hide the checkbox when it’s not multi-select, though the a11y impact would need to be tested.
| <ToggleControl | ||
| __next40pxDefaultSize | ||
| __nextHasNoMarginBottom | ||
| label={ __( 'Show All Posts', 'tribe' ) } | ||
| help={ __( | ||
| 'If checked, all posts will be displayed in the grid. If unchecked, the number of posts displayed will be limited to the number set in the "Posts Per Page" setting.', | ||
| 'tribe' | ||
| ) } | ||
| checked={ showAllPosts } | ||
| onChange={ ( value ) => | ||
| setAttributes( { showAllPosts: value } ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
I’m a little concerned about exposing “Show All Posts” to editors if this sets posts_per_page to -1 under the hood. I’d lean toward removing this option and just letting editors choose a higher capped value for Posts Per Page instead, maybe up to 99. If we ever need truly unlimited behavior, that may be better kept as a developer controlled configuration rather than an editor facing setting.
If we do keep this option, I think we should hide the pagination toggle when showAllPosts is true, since those settings conflict logically.
There was a problem hiding this comment.
Removed the show all posts option and updated the posts per page setting to go up to 99. Agree that -1 can be configured based on the post type by a developer.
| <SortableFacet | ||
| key={ facet.slug } | ||
| facet={ facet } | ||
| onRemove={ handleRemoveFacet } | ||
| onDisplayLabelChange={ | ||
| handleDisplayLabelChange | ||
| } | ||
| /> |
There was a problem hiding this comment.
Really nice SortableFacet component. The drag-and-drop behavior and inline label editing UX are very well done. 🤯 🌮
| } | ||
|
|
||
| .b-facetwp-filter-bar__icon-btn:hover { | ||
| background: var(--wp-admin-theme-color, #007cba); |
There was a problem hiding this comment.
🟡 This variable doesn’t exist. Also, the mobile filter flyout doesn’t appear to handle theme colors yet. I tested it with the group wrapper set to dark, and the labels become unreadable because its text color is the same as the background color.
There was a problem hiding this comment.
--wp-admin-theme-color doesn't exist in Moose, but it does exist in the editor context, added by WP: https://p.tri.be/i/IGVshe
There was a problem hiding this comment.
I also fixed the issue with the flyout not correctly styling the text / button colors.
3 participants
What does this do/fix?
This pull request introduces a new integration with FacetWP, adding three new FacetWP-related blocks and their controllers, as well as a suite of enhancements to block registration, styling, and integration points. The changes include backend PHP logic for handling FacetWP blocks, improved asset enqueuing for block styles and scripts, and various integration hooks for customizing FacetWP's behavior and accessibility. Additionally, there are minor improvements to theme variables and icon sets.
FacetWP Integration and Blocks
tribe/facetwp-archive,tribe/facetwp-filter-bar, andtribe/facetwp-grid, each with dedicated controller classes (FacetWP_Archive_Controller,FacetWP_Filter_Bar_Controller,FacetWP_Grid_Controller) to handle block-specific logic and rendering. [1] [2] [3] [4]FacetWPintegration service with methods for retrieving facets, enhancing accessibility (adding IDs to controls), removing counts from certain facet types, registering custom facets (pagination, search, reset), and customizing pagination markup.Integrations_Subscriberto expose facets to the block editor, modify FacetWP HTML output for accessibility and styling, and register custom facets and pagination markup.Block Asset Registration and Loading
wp_enqueue_style/wp_enqueue_scriptwith proper handles and versioning, and are loaded during the appropriateenqueue_block_assetsaction for both frontend and editor contexts. [1] [2] [3] [4]Block Controller Enhancements
contextproperty, allowing block controllers to access block context values (needed for FacetWP filter bar positioning and logic). [1] [2]Block Editor Template Improvements
Theme and Style Improvements
QA
Links to relevant issues
Screenshots/video:
Pull request checklist